Repository navigation
[feat][pip][consumer] Add Pause/Resume/Paused to Consumer - #1507
PavelZeger wants to merge 9 commits into
Conversation
|
Hi @nodece and @crossoverJie . Appreciate for the review |
There was a problem hiding this comment.
Pull request overview
This PR adds a pause/resume backpressure API to the Go Consumer to match the Java client’s semantics: when paused, the consumer stops issuing flow permits so the broker stops delivering new messages while already-buffered/in-flight messages remain consumable and ack/nack continues to work.
Changes:
- Added
Pause(),Resume(), andPaused()to the publicConsumerinterface with detailed GoDoc describing semantics across subscription types. - Implemented paused state propagation across consumer implementations, and gated flow-permit issuance in
partitionConsumer(including reconnect initial permit handling). - Added integration tests covering pause/resume behavior for standard, partitioned, zero-queue, and regex consumers.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pulsar/consumer.go | Extends the public Consumer interface with Pause/Resume/Paused and documents expected semantics. |
| pulsar/consumer_impl.go | Wires Pause/Resume/Paused through the main consumer and ensures newly discovered partitions inherit paused state. |
| pulsar/consumer_partition.go | Adds per-partition paused flag and gates flow-permit issuance (including reconnect initial grants). |
| pulsar/consumer_multitopic.go | Propagates Pause/Resume/Paused across multi-topic consumers. |
| pulsar/consumer_regex.go | Adds pause state to regex consumer and ensures newly discovered topics inherit paused state. |
| pulsar/consumer_zero_queue.go | Implements Pause/Resume/Paused for zero-queue consumer via its underlying partitionConsumer. |
| pulsar/consumer_test.go | Adds pause/resume tests and helper drain functions used by the new test cases. |
| pulsar/internal/pulsartracing/consumer_interceptor_test.go | Updates a mock consumer to satisfy the expanded Consumer interface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
# Conflicts: # pulsar/consumer_test.go
|
Additional note: there is a known edge case in the partitioned/regex path. If a new partition (or regex topic) is discovered at the exact moment someone calls |
|
Further this conversation I would like to mark this PR as active: https://lists.apache.org/thread/cgy7w0qgnq8s7j0ykyzgrn3hj4nb67wf. |
# Conflicts: # pulsar/consumer_test.go
nodece
left a comment
There was a problem hiding this comment.
I did a deep pass on the concurrency aspects of this change (plus ran the four new tests and the consumer regression suites against a live broker — all green, and I additionally probed the reconnect-while-paused scenario 3x: 0 messages delivered while paused, exact remainder after Resume(), so the dispatcher stash works).
What I verified as correct
flowIfNeedis indeed the single permit-return path (checked everyavailablePermits.add/inccall site); the dispatcher initial grant is the one exception and is correctly stashed.flush()andflowIfNeed()both claim permits viapermits.CompareAndSwap(current, 0)— concurrent execution can neither double-send nor lose permits.resume()'sCompareAndSwap(true, false)guards against double-flush, and the re-check ofpausedinsideflush()handles the resume→pause interleaving correctly.internalFlowfrom an application goroutine is safe — the zero-queueReceivepath already does this today.
Nit (non-blocking): top-level Pause/Resume are multi-step and not serialized
func (c *consumer) Pause() {
c.paused.Store(true)
for _, pc := range c.partitionConsumers() { pc.pause() }
}Concurrent Pause()/Resume() on the same consumer can interleave as: top-level flag false, partition flag true, flush skipped — i.e. Paused() reports resumed while delivery is actually paused. It self-heals on the next call and requires the application to race the two control calls, so I don't think it should block this PR (the Java client is similarly unsynchronized), but if you want to harden it:
CompareAndSwapinpause()would not help —pause()has no side effect to guard, and CAS cannot atomize the "flag + N partitions" multi-step operation.- A channel/actor loop would serialize it but costs a goroutine per consumer, close semantics, and a done-ack to preserve the current "Pause() returns → paused" guarantee.
The proportionate fix is a control-plane mutex that serializes the writers while keeping the readers lock-free:
type consumer struct {
paused atomic.Bool // readers (flowIfNeed, Paused()) stay lock-free
pauseMu sync.Mutex // serializes Pause/Resume writers
}
func (c *consumer) Pause() {
c.pauseMu.Lock()
defer c.pauseMu.Unlock()
c.paused.Store(true)
for _, pc := range c.partitionConsumers() { pc.pause() }
}
// Resume() symmetricTaking the same pauseMu in internalTopicSubscribeToPartitions would additionally close the partition-discovery inheritance race (discovery checks c.paused per new partition on a background goroutine, which can also race a concurrent Pause()).
Test suggestion (also non-blocking)
The current tests don't cover reconnecting while paused — the most subtle path of the change (the stash). A cheap test: pause, pc._getConn().Close(), assert the backlog does not fully drain while paused, then Resume() and drain the exact remainder. Happy to contribute the probe I used if useful.
|
Thank you, @nodece , for the review. Do you prefer all fixes to push into this branch or approve now and I'll open a new dedicated branch with fixes? |
Fixes #1504
Motivation
The Java client exposes
pause()/resume()on everyConsumer; the Go client has no equivalent. While paused, a consumer should stop issuing flow permits to the broker so no newmessages are delivered, while already-buffered, in-flight, and previously-granted messages remain available via
Receive()/Chan()and ack/nack keep working.Pause/resume is the standard backpressure pattern for when a downstream is temporarily unavailable (database failover, a "halt ingestion" feature flag, orderly shutdown that finishes in-flight work before closing). The only workaround today is closing and re-subscribing, which is heavyweight (re-resolves the topic, redelivers unacked messages, and can rebalance other consumers in
KeyShared/Failover).The semantics were reviewed and agreed in #1504.
Modifications
Pause(),Resume(), andPaused()to theConsumerinterfacepulsar/consumer.go), with GoDoc matching the Java semantics.partitionConsumer(pulsar/consumer_partition.go):pausedflag and gated permit flow inflowIfNeed— the single place that returns permits to the broker.availablePermitsinstead of sending it, so reconnecting while paused does not resume delivery.resume()flushes only the permits actually owed (viaflowIfNeed), mirroring Java'sincreaseAvailablePermits(cnx, 0), rather than granting a fresh full batch (which would over-grant after a mid-stream pause).Consumerimplementations:consumer,zeroQueueConsumer,multiTopicConsumer, andregexConsumer.Paused()is reported at the top level ("Pause()called more recently thanResume()").internalTopicSubscribeToPartitions) and newly discovered regex topics (regexConsumer.subscribe) inherit the parent's paused state at creation.Receive()is unchanged: the existing permit is gated while paused, soReceive(ctx)blocks untilResume()(or the context is cancelled).Deviations from the original PIP
The observable behavior agreed in #1504 is unchanged; four implementation details differ from the issue's wording.
Resume()flushes only the permits actually owed (and the dispatcher stashes the initial grant while paused) instead of sending a fullcurrentQueueSizebatch, which would over-grant after a mid-stream pause. The zero-queue consumer reuses the same flow gate instead of a newresumedCh. The getter is namedPaused()rather thanIsPaused().BatchReceiveis out of scope, since the Go client doesn't have that method yet.Verifying this change
This change added tests and can be verified as follows:
TestConsumerPauseResume,TestPartitionedConsumerPauseResume,TestZeroQueueConsumerPauseResume, andTestRegexConsumerPauseResumeInheritsNewTopicsinpulsar/consumer_test.go. They verify that a paused consumer stops delivery before draining the full backlog, thatResume()delivers the remainder with no message loss, thatPause()/Resume()are idempotent, thatPaused()reflects state, that a newly discovered regex topic inherits the paused state, and that a zero-queueReceive()blocks while paused and returns after resume.Does this pull request potentially affect one of the following parts:
Pause(),Resume(), andPaused()to theConsumerinterfaceCommandFlow; pausing simply withholds it)Documentation
Consumerinterface methods)